*: cp 67857 #68244
Conversation
ti-chi-bot
Bot
added
release-note-none
|
Hi @windtalker. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
📝 WalkthroughWalkthroughThis PR introduces a session-level prepared-statement deduplication cache that avoids redundant full preparations by caching and rebuilding from template metadata. A new ChangesPrepared Statement Deduplication Cache
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.1)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/planner/core/plan_cache_utils.go (1)
661-664: ⚡ Quick winMake
CollectPlanCacheStmtInfoidempotent before AST walk.
CollectPlanCacheStmtInfocurrently appends into existingstmt.limits/stmt.tablesand keepshasSubquerysticky. Ifstmtis reused, stale metadata can accumulate and skew later cache-key/stat-related behavior. Reset the derived fields before collecting.Proposed patch
func CollectPlanCacheStmtInfo(ctx context.Context, is infoschema.InfoSchema, stmt *PlanCacheStmt, stmtNode ast.StmtNode) { + stmt.limits = nil + stmt.tables = nil + stmt.hasSubquery = false processor := &planCacheStmtProcessor{ctx: ctx, is: is, stmt: stmt} stmtNode.Accept(processor) }As per coding guidelines, code SHOULD remain maintainable for future readers and avoid subtle state-coupling pitfalls.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/planner/core/plan_cache_utils.go` around lines 661 - 664, CollectPlanCacheStmtInfo should clear derived state on the provided *PlanCacheStmt before walking the AST: reset stmt.limits and stmt.tables to empty slices (e.g., nil or freshly allocated empty slices) and set stmt.hasSubquery = false so repeated calls don't accumulate stale data; do this at the start of CollectPlanCacheStmtInfo just before creating the planCacheStmtProcessor and calling stmtNode.Accept(processor).pkg/server/internal/testserverclient/server_client.go (1)
2584-2587: ⚡ Quick winConsider adding a comment to explain the variable override.
The code correctly disables
tidb_enable_cache_prepare_stmtto ensure the test's error/warning counting logic remains stable, but a brief comment would help future maintainers understand why this override is necessary.📝 Suggested documentation improvement
+ // Disable prepared-statement caching to ensure consistent error/warning counts + // in information_schema tables during the test dbt.MustExec("set @@tidb_enable_cache_prepare_stmt = off") defer func() { dbt.MustExec("set @@tidb_enable_cache_prepare_stmt = default") }()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/server/internal/testserverclient/server_client.go` around lines 2584 - 2587, Add a brief inline comment above the dbt.MustExec call that disables tidb_enable_cache_prepare_stmt explaining that the test disables cached prepared statements to ensure stable error/warning counting across repeated prepared executions, and note that the deferred call restores the setting to default; reference the dbt.MustExec("set @@tidb_enable_cache_prepare_stmt = off") call and its deferred dbt.MustExec(... = default) to make the intent and need for the override clear to future maintainers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/session/test/common/prepare_dedup_cache_test.go`:
- Around line 162-169: The test's DB-isolation check is too weak: after calling
tk.Session().PrepareStmt(sql) for db2 we only assert require.Len(t, fields2, 1)
which won't catch reuse of the db1 cache entry; update the test to verify that
the prepared metadata actually reflects db2 (not db1) by asserting the column
type or behavior. Specifically, after PrepareStmt(sql) for db2 (where id2 is
returned), add an assertion on fields2[0].Column.GetType() (or equivalent type
field) equals the bigint/mysql.TypeLonglong constant from
"github.com/pingcap/tidb/pkg/parser/mysql", or alternatively execute the
prepared statement against db2 data and assert the numeric result matches
db2.tblx.v; keep the existing require.NotEqual(t, id1, id2) and import the mysql
package if you choose the type assertion approach.
---
Nitpick comments:
In `@pkg/planner/core/plan_cache_utils.go`:
- Around line 661-664: CollectPlanCacheStmtInfo should clear derived state on
the provided *PlanCacheStmt before walking the AST: reset stmt.limits and
stmt.tables to empty slices (e.g., nil or freshly allocated empty slices) and
set stmt.hasSubquery = false so repeated calls don't accumulate stale data; do
this at the start of CollectPlanCacheStmtInfo just before creating the
planCacheStmtProcessor and calling stmtNode.Accept(processor).
In `@pkg/server/internal/testserverclient/server_client.go`:
- Around line 2584-2587: Add a brief inline comment above the dbt.MustExec call
that disables tidb_enable_cache_prepare_stmt explaining that the test disables
cached prepared statements to ensure stable error/warning counting across
repeated prepared executions, and note that the deferred call restores the
setting to default; reference the dbt.MustExec("set
@@tidb_enable_cache_prepare_stmt = off") call and its deferred dbt.MustExec(...
= default) to make the intent and need for the override clear to future
maintainers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0b6683e2-2eae-403b-8f0f-20f9e09e6c35
📒 Files selected for processing (11)
pkg/planner/core/plan_cache_utils.gopkg/server/internal/testserverclient/server_client.gopkg/session/session.gopkg/session/test/common/BUILD.bazelpkg/session/test/common/prepare_dedup_cache_test.gopkg/sessionctx/variable/session.gopkg/sessionctx/variable/setvar_affect.gopkg/sessionctx/variable/sysvar.gopkg/sessionctx/variable/tidb_vars.gotests/integrationtest/r/sessionctx/setvar.resulttests/integrationtest/t/sessionctx/setvar.test
| // Prepare in db2 — must NOT hit the db1 cache entry because currentDB differs. | ||
| tk.MustExec("use db2") | ||
| id2, _, fields2, err := tk.Session().PrepareStmt(sql) | ||
| require.NoError(t, err) | ||
| require.NotEqual(t, id1, id2) | ||
| // fields2 reflects db2.tblx.v which is bigint (not int), so a fresh build must have run. | ||
| require.Len(t, fields2, 1) | ||
| } |
There was a problem hiding this comment.
Strengthen the DB-isolation assertion — current check doesn't actually verify isolation.
The comment claims "fields2 reflects db2.tblx.v which is bigint (not int), so a fresh build must have run", but the only assertion is require.Len(t, fields2, 1). That length check would pass identically if the dedup cache had (incorrectly) reused the db1 entry — both schemas have a single column. The test will not catch a regression where dedup keys ignore CurrentDB.
Tighten the check by asserting on the column type or by executing the prepared stmt against db2 data and verifying the result.
🔧 Suggested stronger assertion
// Prepare in db2 — must NOT hit the db1 cache entry because currentDB differs.
tk.MustExec("use db2")
id2, _, fields2, err := tk.Session().PrepareStmt(sql)
require.NoError(t, err)
require.NotEqual(t, id1, id2)
- // fields2 reflects db2.tblx.v which is bigint (not int), so a fresh build must have run.
require.Len(t, fields2, 1)
+ // fields2 must reflect db2.tblx.v (bigint), not the cached db1 schema (int);
+ // otherwise dedup is incorrectly ignoring CurrentDB.
+ require.Equal(t, mysql.TypeLonglong, fields2[0].Column.FieldType.GetType(),
+ "db2.tblx.v should be BIGINT; a stale db1 cache hit would return INT")
+ require.Equal(t, mysql.TypeLong, fields1[0].Column.FieldType.GetType(),
+ "sanity: db1.tblx.v is INT")You'll need to import "github.com/pingcap/tidb/pkg/parser/mysql".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/session/test/common/prepare_dedup_cache_test.go` around lines 162 - 169,
The test's DB-isolation check is too weak: after calling
tk.Session().PrepareStmt(sql) for db2 we only assert require.Len(t, fields2, 1)
which won't catch reuse of the db1 cache entry; update the test to verify that
the prepared metadata actually reflects db2 (not db1) by asserting the column
type or behavior. Specifically, after PrepareStmt(sql) for db2 (where id2 is
returned), add an assertion on fields2[0].Column.GetType() (or equivalent type
field) equals the bigint/mysql.TypeLonglong constant from
"github.com/pingcap/tidb/pkg/parser/mysql", or alternatively execute the
prepared statement against db2 data and assert the numeric result matches
db2.tblx.v; keep the existing require.NotEqual(t, id1, id2) and import the mysql
package if you choose the type assertion approach.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: guo-shaoge, xzhangxian1008 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
|
@windtalker: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/override fast_test_tiprow_for_release idc-jenkins-ci-tidb/check_dev idc-jenkins-ci-tidb/check_dev_2 idc-jenkins-ci-tidb/mysql-test idc-jenkins-ci-tidb/unit-test |
|
@timzxz: Overrode contexts on behalf of timzxz: fast_test_tiprow_for_release, idc-jenkins-ci-tidb/check_dev, idc-jenkins-ci-tidb/check_dev_2, idc-jenkins-ci-tidb/mysql-test, idc-jenkins-ci-tidb/unit-test DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
ti-chi-bot
Bot
merged commit 9984795
into
pingcap:feature/release-8.5-materialized-view-2603
5 participants
What problem does this PR solve?
Issue Number: close #xxx
Problem Summary:
What changed and how does it work?
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit
Release Notes
New Features
tidb_enable_cache_prepare_stmtsystem variable (enabled by default) to cache prepared statement metadata within a session when the same SQL is prepared multiple times.Tests